Add get_dist_dependency_conflicts back to opentelemetry-instrumentation
#3418
Conversation
opentelemetry-instrumentation/src/opentelemetry/instrumentation/dependencies.py
Show resolved
Hide resolved
|
Needs changelog as well |
|
azure-monitor-opentelemetry==1.6.7 was changed to use the new dependency system instead of this. So, that package no longer needs this. However, I am still open to adding this back in since I don't think it should have been removed. We can discuss in the SIG. |
|
Additional note, I realized our Azure Auto-Instrumentation would really benefit from having this back as well. There are scenarios where you may want to check whether a library is installed separate from instrumenting it. Example: We want to check if DJANGO_SETTINGS_MODULE is set if and only if Django is installed. If it's not installed, there's no need. If it is installed, we want to know that BEFORE attempting to instrument Django. |
|
Further explanation for why we need this back With the new approach, there will always be an ERROR log whenever there is a dependency conflict. So, running autoinstrumentation now produces tons of instrumented error logs for every package not installed. |
|
More support for this PR: Calling distro.load_instrumentor will generally not raise DependencyConflictError. First, load_instrumentor loads the entrypoint which will result in a ModuleNotFound error if the instruemnted library does not exist. This occurs before instrument() is even called let alone when it has the chance to raise a DependencyConflictError |
|
@jeremydvoss About your first concern, do you have a situation as an example? About your second concern, do you think 9c969f3 works for you? |
No, it should not be a ModuleNotFound. It is a dependency conflict. In other words, that DependencyConflictException section does not do anything. We could solve it by adding the dependency check to distro.load_instrument. I'll work on that as a separate PR. |
Signed-off-by: emdneto <[email protected]>
Signed-off-by: emdneto <[email protected]>
| ) -> DependencyConflict | None: | ||
| warnings.warn( | ||
| "get_dist_dependency_conflicts is deprecated since 0.53b0 and will be removed in a future release.", | ||
| DeprecationWarning, |
There was a problem hiding this comment.
Don't add deprecation warning until we are sure of our path forward.
There was a problem hiding this comment.
As we discussed in slack: let's remove this.
3 participants
Description
Closes #3415